Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: headless nav toggle #860

Merged
merged 11 commits into from
Oct 19, 2021
Merged

feat: headless nav toggle #860

merged 11 commits into from
Oct 19, 2021

Conversation

josephaxisa
Copy link
Contributor

Added a toggle for when APIX is running in headless mode. Screenshots don't really help, I suggest trying this locally.

@google-cla google-cla bot added the cla: yes label Oct 15, 2021
@github-actions
Copy link
Contributor

APIX Tests

    1 files  ±0    80 suites  ±0   2m 30s ⏱️ -27s
320 tests ±0  307 ✔️ ±0  13 💤 ±0  0 ❌ ±0 
336 runs  ±0  323 ✔️ ±0  13 💤 ±0  0 ❌ ±0 

Results for commit 23af69e. ± Comparison against base commit 55daf45.

jkaster
jkaster previously requested changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

packages/api-explorer/src/components/Header/Header.tsx Outdated Show resolved Hide resolved
@josephaxisa @jkaster
Update packages/api-explorer/src/components/Header/Header.tsx
387967a 
Co-authored-by: John Kaster <kaster@google.com>
@github-actions
Copy link
Contributor

APIX Tests

    1 files  ±0    80 suites  ±0   3m 22s ⏱️ +25s
320 tests ±0  307 ✔️ ±0  13 💤 ±0  0 ❌ ±0 
336 runs  ±0  323 ✔️ ±0  13 💤 ±0  0 ❌ ±0 

Results for commit 387967a. ± Comparison against base commit 55daf45.

@github-actions
Copy link
Contributor

APIX Tests

    1 files    80 suites   2m 56s ⏱️
320 tests 307 ✔️ 13 💤 0 ❌
336 runs  323 ✔️ 13 💤 0 ❌

Results for commit 2e5d1e2.

@github-actions
Copy link
Contributor

APIX Tests

    1 files    80 suites   2m 34s ⏱️
320 tests 307 ✔️ 13 💤 0 ❌
336 runs  323 ✔️ 13 💤 0 ❌

Results for commit 8de1160.

jkaster
jkaster previously approved these changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jhardy
jhardy previously requested changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@jhardy jhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small suggest, smaller sidebar on collapsed state and better centering when collapsed

packages/api-explorer/src/ApiExplorer.tsx Outdated Show resolved Hide resolved
packages/api-explorer/src/ApiExplorer.tsx Outdated Show resolved Hide resolved
@josephaxisa @jhardy
Update packages/api-explorer/src/ApiExplorer.tsx
8b4bacc 
Co-authored-by: Jared <hardy.jared@gmail.com>
@github-actions
Copy link
Contributor

APIX Tests

    1 files    80 suites   3m 15s ⏱️
320 tests 307 ✔️ 13 💤 0 ❌
336 runs  323 ✔️ 13 💤 0 ❌

Results for commit dc81cda.

jkaster
jkaster requested changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I put headless: true in packages/api-explorer/public/versions.json it displays correctly and works.

However, if I've configured a remote server to OAuth to, the headless: true value is lost, no matter what I do, even if I hard code it. So there's something (probably not your fault) that is breaking it for its intended usage when embedded in the developer portal.

@josephaxisa
Copy link
Contributor Author

I just tested it locally and can confirm the same result but this PR does not touch that logic. I am seeing the same issue on main. Hardcoding headless={true} results in everything working as expected.

sdk-codegen/packages/api-explorer/src/StandaloneApiExplorer.tsx

Lines 105 to 110 in 31fe97c

<ApiExplorer
specs={specs}
envAdaptor={standaloneEnvAdaptor}
headless={embedded}
setVersionsUrl={setCurrentVersionsUrl}
/>

Looking into this briefly I can see that on initial load we are reading versions from the statically served versions.json that's bundled with API Explorer. After a successful OAuth flow, the /versions endpoint on the API server is called and it is expected to contain the headless property which doesn't sound right, please correct me if I'm wrong.

Perhaps the indicator of whether an instance of API Explorer is headless or not should be determined at the usage site by passing in the prop headless={true}. I don't think there's a need to persist it in local storage either. Right now the prop value is getting overridden because of the absence of the headless prop from the /versions payload.

@github-actions
Copy link
Contributor

APIX Tests

    1 files    80 suites   2m 55s ⏱️
320 tests 307 ✔️ 13 💤 0 ❌
336 runs  323 ✔️ 13 💤 0 ❌

Results for commit 8458409.

@github-actions
Copy link
Contributor

APIX Tests

    1 files    80 suites   2m 54s ⏱️
320 tests 307 ✔️ 13 💤 0 ❌
336 runs  323 ✔️ 13 💤 0 ❌

Results for commit 8ead213.

@josephaxisa josephaxisa dismissed jhardy’s stale review October 19, 2021 17:07

changes have been accepted.

@josephaxisa josephaxisa merged commit 8e66d1b into main Oct 19, 2021
@josephaxisa josephaxisa deleted the jax/headless-nav-toggle branch October 19, 2021 17:08
@github-actions

This comment has been minimized.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkaster jkaster jkaster approved these changes

@bryans99 bryans99 Awaiting requested review from bryans99

@jhardy jhardy Awaiting requested review from jhardy

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@josephaxisa @jhardy @jkaster